Fix NullReferenceException in tool install when package not found in manifest#52951
Fix NullReferenceException in tool install when package not found in manifest#52951
Conversation
Co-authored-by: marcpopMSFT <12663534+marcpopMSFT@users.noreply.github.com>
Co-authored-by: marcpopMSFT <12663534+marcpopMSFT@users.noreply.github.com>
|
@copilot I see that you modified ToolInstallLocalCommand. Wouldn't it be better to fix the shared method that is ExplicitManifestOrFindManifestContainPackageId to handle this situation rather than duplicating some of that code? |
Co-authored-by: marcpopMSFT <12663534+marcpopMSFT@users.noreply.github.com>
You're absolutely right. I've refactored the fix to modify the shared The method now accepts an optional
Commit: 0c4ffbf |
|
Before After |
|
Same for local install. |
|
@copilot do we have any tests covering tool installation that should have detected this? If not, please create one so we don't regress this ability. |
There was a problem hiding this comment.
Pull request overview
Fixes a regression in dotnet tool install where missing tool manifest discovery could surface as an unhandled exception, by allowing install to proceed and create/select a manifest when appropriate.
Changes:
- Added an optional
throwIfNoManifestFoundparameter toExplicitManifestOrFindManifestContainPackageIdto optionally suppress manifest-not-found exceptions. - Updated local tool install to call the helper with
throwIfNoManifestFound: falseand fall back toGetManifestFilePath()when no manifest is found/selected.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Cli/dotnet/Commands/Tool/Install/ToolInstallLocalCommand.cs | Uses the updated helper with throwIfNoManifestFound: false and simplifies manifest selection/fallback logic. |
| src/Cli/dotnet/Commands/Tool/Common/ToolManifestFinderExtensions.cs | Adds optional behavior to suppress throwing when no manifest exists, enabling install to handle creation/fallback. |
| public static (FilePath? filePath, string warningMessage) ExplicitManifestOrFindManifestContainPackageId( | ||
| this IToolManifestFinder toolManifestFinder, | ||
| string explicitManifestFile, | ||
| PackageId packageId) | ||
| PackageId packageId, | ||
| bool throwIfNoManifestFound = true) |
There was a problem hiding this comment.
warningMessage is declared as non-nullable (string) in the tuple return type, but this method returns null for it in several paths (explicit manifest, no match, and now the no-manifest catch). This makes the nullability contract incorrect for nullable-enabled callers and can lead to missed null checks / NRT warnings. Consider enabling nullable for this file (or at least for this method) and changing the signature to return string? warningMessage (and likewise accept string? explicitManifestFile).
There was a problem hiding this comment.
Fixed the nullability annotations:
- Removed
#nullable disablefrom ToolManifestFinderExtensions.cs - Changed return type to
(FilePath? filePath, string? warningMessage) - Changed parameter to
string? explicitManifestFile - Updated calling code in ToolInstallLocalCommand.cs to use
string?
All tests pass (21 install + 9 uninstall).
Commit: 4921020
| (FilePath? manifestFileOptional, string warningMessage) = | ||
| _toolManifestFinder.ExplicitManifestOrFindManifestContainPackageId(_explicitManifestFile, packageId); | ||
| _toolManifestFinder.ExplicitManifestOrFindManifestContainPackageId(_explicitManifestFile, packageId, throwIfNoManifestFound: false); | ||
|
|
||
| if (warningMessage != null) | ||
| { | ||
| _reporter.WriteLine(warningMessage.Yellow()); |
There was a problem hiding this comment.
In this nullable-enabled file, warningMessage is deconstructed as non-nullable string, but the called helper can return null for the warning. This forces a redundant warningMessage != null check (and can hide real nullability issues). After adjusting the helper’s signature, consider deconstructing into string? warningMessage here to match the actual behavior.
There was a problem hiding this comment.
This was already addressed in commit 4921020. Line 105 now uses string? warningMessage to properly match the nullable return type from the helper method.
Co-authored-by: baronfel <573979+baronfel@users.noreply.github.com>
Yes, we have existing unit tests that cover tool installation scenarios. The test However, I've added a more explicit regression test Commit: 14280f6 |
Co-authored-by: marcpopMSFT <12663534+marcpopMSFT@users.noreply.github.com>
|
This is not what we should do, please see the commit I made in #53001 |
|
fixed by #53001 |
Fix
dotnet tool installNullReferenceException RegressionSummary
Fixes a critical regression in .NET 11.0.100-preview.2 where
dotnet tool installwould fail with "Unhandled exception: Object reference not set to an instance of an object."Solution
Modified the shared
ExplicitManifestOrFindManifestContainPackageIdextension method to accept an optionalthrowIfNoManifestFoundparameter:falseto allow manifest creation when none existstruemaintains existing behavior (throws if no manifest found)This approach avoids code duplication and centralizes the manifest lookup logic in the shared method.
Changes
ToolManifestFinderExtensions.cs:throwIfNoManifestFoundparameter (defaults totrue)#nullable disableand properly annotated nullable types(FilePath? filePath, string? warningMessage)string? explicitManifestFileToolInstallLocalCommand.cs:throwIfNoManifestFound: falsestring?forwarningMessageToolUninstallLocalCommand.cs: No changes needed (uses defaulttruebehavior)ToolInstallLocalCommandTests.cs: Added explicit regression testTesting
GivenExistingEmptyManifestItShouldInstallToolSuccessfullyspecifically covers the regressionRisk Assessment
Low - Cleaner solution that leverages existing shared code with explicit regression coverage and proper nullability
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.